wallet2_basic: robust dep-free lib for loading/storing wallet2 files #8923
Conversation
jeffro256
force-pushed
the
wallet2_conv
branch
2 times, most recently
from
5357cc0
to
ba5a9a3
Compare
|
What is considered a "legacy wallet" ? |
|
I mean the current wallet2 wallets. I used the term “legacy” in the context of the new wallet code being worked on in the “No Wallet Left Behind” project |
There was a problem hiding this comment.
I'm not really a fan of changing serialization formats to json. Was this discussed in the chatroom you mentioned (I'm now in that room)?
Msgpack is better for perf and size reasons, especially if string keys are used its trivial to generate a JSON tree with a generic implementation.
| namespace serialization | ||
| { | ||
| template <class Archive> | ||
| inline void serialize(Archive &a, wallet2_basic::transfer_details &x, const boost::serialization::version_type ver) |
There was a problem hiding this comment.
This might be a good time to try boost::polymorphic_oarchive and boost::polymorphic_iarchive. It should be possible to put these implementations in a cpp - the slowdown from the stable calls should be minimal.
There was a problem hiding this comment.
Realistically, there's basically only ever two archive classes that will ever be used here: {portable_}binary_iarchive so I put those definitions into a cpp file but wrote it so that this can easily be extended to other templated archives or polymorphic archives.
There was a problem hiding this comment.
Why not use the polymorphic type now? Does it not work? There will be a speed difference, but its should be somewhat small.
There was a problem hiding this comment.
I'm trying it out right now, but one unfortunate thing for binary bloat if we go the polymorphic route is that there is no Boost provided polymorphic route for portable_binary_iarchive so we have to define the class and generate the code for this route in the wallet binary.
| } | ||
|
|
||
| template <typename T> | ||
| void read_json_object_key_Int(T& out, const rapidjson::Value& json, const char* name, bool mand) |
There was a problem hiding this comment.
Is all this new serialization stuff? Why rapidjson all of a sudden? I have a msgpack implemenation ready with unit tests.
There was a problem hiding this comment.
I answered my own question eventually, this isn't just moving around files @UkoeHB this is changing the serialization format to a JSON format backed by rapidjson.
There was a problem hiding this comment.
My understanding is that this library is only intended to be used to read old wallet formats, so we can drop wallet2 while keeping support for old formats. None of this is new, .keys files use JSON today.
There was a problem hiding this comment.
@tobtoht is correct. This lib defines new types cache and keys_data and has analogous deserialization code to wallet2::load. The generally point of the lib is to allow the core project to support the current wallet formats moving forwards, but ditch the vast majority of wallet2 code.
There was a problem hiding this comment.
I don't see (at a quick glance) where the code is "coming from" (the corresponding delete).
I also don't see a store function for keys_data, was that left untouched?
jeffro256
force-pushed
the
wallet2_conv
branch
6 times, most recently
from
1b6e1f2
to
9433232
Compare
|
Another benefit of this PR that I contributed as a side effect of recalculating the cache key at loading/storing: the EDIT: I reverted this change since the way the wallet is structured, it would be way too much refactoring to change automatic storing without |
| else | ||
| ASSERT_MES_AND_THROW("Field " << name << " found in JSON, but not " << "Int"); | ||
| } | ||
| else if (mand) |
There was a problem hiding this comment.
I think the value should be default initialized when not mandatory.
There was a problem hiding this comment.
I have the defaults listed in the header wallet2_storage.h for each field in the class. Since we start deserializing with a default constructed object, if the field is not present in the JSON, then the default constructed value stays.
| } | ||
|
|
||
| template <typename T> | ||
| void read_json_object_key_Int(T& out, const rapidjson::Value& json, const char* name, bool mand) |
There was a problem hiding this comment.
I don't see (at a quick glance) where the code is "coming from" (the corresponding delete).
I also don't see a store function for keys_data, was that left untouched?
Yes, but I'm planning to integrate it in actually just because it will help us "prove" that the code works. The load/storing keys data code should be much more efficient as well anyways |
jeffro256
force-pushed
the
wallet2_conv
branch
4 times, most recently
from
66395f9
to
0ac8189
Compare
jeffro256
changed the title
jeffro256
changed the title
jeffro256
changed the title
|
For the reviewers' sake should I split this into multiple PRs: One for adding the code, the other for integration? |
|
@vtnerd I used the polymorphic archives with this latest commit and was able to shrink the binary size by 3% |
This library has no dependency on wallet2.h and gives us a way forward to move away from `wallet2` in the (not-so-distant) future, while still supporting conversions of old wallet files. This lib is also useful if you have an application where you want to extract information directly from the wallet file with or without having to setup accounts and devices. This is now possible because I have split the wallet keys loading into two steps: `load_from_memory` and `setup_account_and_devices`. When one is loading a wallet keys file, the user of the API can choose whether or not to contact external devices during this process with use of the flag `allow_external_devices_setup`.
|
I'm moving this PR to the seraphis-migration repository here: seraphis-migration#4 |
|
Why move it there? Isn't that repo going to eventually PR back into this repo? |
|
The reason I chose to do it this way is so that I can quickly prototype and reiterate on the code as use cases arise, since that repo needs these changes more immediately than the core repo and there's fewer review requirements. Then my plan was to PR against the main repo when the code was in a more concrete form. But honestly, I'd probably be swayed pretty easily if you convinced me to PR it here first then downstream it. |
If you ask me you can't develop code in two places at once. Either let it flow into the main repo, master branch, and improve it further with more PRs to the master branch, or PR it to the Seraphis repo and then it lives there and gets developed further there. Either way works for me. A decision soon would be nice however, because I wait for the moment you declare "That's it now, please review". |
This library has no dependency on wallet2.h and gives us a way forward to move away from
wallet2in the (not-so-distant) future, while still supporting conversions of old wallet files. This lib is also useful if you have an application where you want to extract information directly from the wallet file with or without having to setup accounts and devices. This is now possible because I have split the wallet keys loading into two steps:load_from_memoryandsetup_account_and_devices. When one is loading a wallet keys file, the user of the API can choose whether or not to contact external devices during this process with use of the flagallow_external_devices_setup.This PR is a work in progress because more testing is needed, though most of the functionality that is needed of it is already implemented.